perf(codegen,runtime): inline precheck for boxed class-field stores, arguments-registry emptiness latch, declared-type refinement for property reads (interp 1.236 -> 1.097, iso_miss 1.670 -> 1.465) - #7854
Conversation
…arguments-registry emptiness latch, declared-type refinement for property reads Round 4 of the `interp` campaign. Three independent changes: A. `expr/property_set.rs` gated the #5093 inline shape precheck on `requires_raw_f64`, so every store into a declared field that is not a `number` paid an unconditional `js_typed_feedback_class_field_set_guard` call — including the synthesized `__AnonShape_*_constructor` behind every closed-shape object literal. The sloppy arm has taken the boxed precheck since #7288 and its argument applies verbatim: the write barrier, layout note and string demote come from `emit_jsvalue_slot_store_pointer_tested`, not from the guard, and a setter in the chain is refused upstream by `class_field_global_index`. Every miss still reaches the unchanged guard call and strict fallback. B. `is_arguments_object` gets the #7474/#7469 emptiness latch. It is a probe run from the by-name property-get tail, array push, the iterator entries and class construction; in a program with no `arguments` it was 2.8% of `interp` — a thread-local resolution (a real `_tlv_get_addr` on Darwin), a `RefCell` borrow and a pointer hash to prove a feature's absence. C. `refine_type_from_init` recovers a property read's type from the receiver's declared annotation through the class / interface / object-alias tables `static_type_of` already consults, after stripping nullish union arms — so `const names = e.names` on `let e: Env | null` stops being `Any` and `names[i]` stops being a `js_dyn_index_get` call. That type is a CLAIM, not a proof. Element reads and stores re-check `GC_TYPE_ARRAY` and tolerate a violated one; `.length` does not, because its `js_value_length_f64` fallback answers 0 where JS answers `undefined`. `declared_only_array_locals` (the #7773 mechanism) keeps these ids off the `.length` fast arm. The sabotage test was written first and failed, which is how that was found. Refutes the round-3 handoff's primary lever: hand-written shape narrowing after a discriminant test converts 19 of 31 generic property diamonds in `evalNode` and is worth 5.2%, not the projected ~20% — the cost is the guard, not the lookup, so narrowing swaps one guarded diamond for another. Claude-Session: https://claude.ai/code/session_012B8z92S82sCfqCrVqrFgS2
|
Warning Review limit reached
Next review available in: 3 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (15)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Gap-suite status — partial, and I could not get a quiet hostStating this plainly rather than claiming a clean sweep. The suite was run locally against this branch's compiler Non-passing tests observed, all triaged:
The four network tests. They SIGABRT on a Rust panic inside What is outstanding: a full gap run on a quiet host. Nothing in the 384 tests The parts of validation that did complete cleanly are in the PR body: the 19-program |
Gap-suite update — final tally for the partial runStopped at 404 / 537 (the box hit load 115 and the run had slowed to ~1 test/min; 10 already in 1 cleared: 6 network/server tests I could not A/B, all Bottom line unchanged: no new failure in the 404 tests observed is attributable to |
Correction: the partial gap run recorded 412/537, not 404My earlier two comments quoted 404 — that was the last line I read out of the live Nothing else changes. The set of non-passing tests is exactly the 17 already Two things worth knowing for whoever finishes it on a quiet host:
|
…ld per construction (shapes 0.183 -> 0.146; isolated probe 1.81x) (#7861) * perf(class): widen the class-field shape guard to the subclass closure WIP * wip: temporary class-field guard miss counters (to be reverted) * wip: keep pre-widening and-chain when the closure is empty (byte-identical IR) * perf(class): declare the typed shape at allocation for subclassed chains (#7512 followup) * Revert "wip: temporary class-field guard miss counters (to be reverted)" * test(codegen): pin the chain prologue analysis * docs: changelog fragment for the class-field chain fix * docs: key the changelog fragment to PR #7861 * docs: record the #7854 boxed-store interaction in the fragment
…et a declared array type reach the guarded element read and the inline .length (#7890) * perf(codegen): interp 0.780 -> 0.675 s, iso_miss 1.061 -> 0.967 s — let a declared array type reach the guarded element read and the inline .length Two halves of one mechanism: what a program may do with an array type that came from an annotation rather than from an initializer that proved an array. A. `e.vals[i]` / `p.toks[p.pos]`. #7854 recovered a receiver's declared property type for a LOCAL (`const names = e.names`), never for the read used directly as a receiver — the HIR types a PropertyGet off a UNION receiver as `Any`, so `index_get.rs` routed those to `js_dyn_index_get`. The tier this unlocks, `lower_guarded_array_index_get`, re-checks GC_TYPE_ARRAY, forwarding, descriptors, the prototype latch and the bounds on the receiver itself, so a violated claim costs a branch and returns the same answer. B. `.length` no longer refuses a declared-only array local. #7854 refused them because the arm's fallback was `js_value_length_f64`, which answered 0 where JS answers `undefined` and did not throw on a nullish receiver (#7853). #7862 replaced that fallback with `js_value_length_property_f64` and left the refusal standing. `declared_only_array_locals` and `refined_array_type_is_declared_only` are deleted with it; `declared_only_numeric_locals` (#7773) is untouched. Quiet M1 mini, best-of-5, exit-checked: interp 0.7796 -> 0.6748 (-13.4%), iso_miss 1.0607 -> 0.9670 (-8.8%). 17 of the 19 corpus programs compile byte-identically and the two that differ are exactly the two with a type-alias over an array; noise floor from those 17 is +-1.0%. Claude-Session: https://claude.ai/code/session_012B8z92S82sCfqCrVqrFgS2 * test(gap): cover a declared-array PropertyGet used directly as an element-read receiver Claude-Session: https://claude.ai/code/session_012B8z92S82sCfqCrVqrFgS2 --------- Co-authored-by: Ralph Küpper <ralph3@skelpo.com>
Round 4 of the
interpcampaign (3.96 s → 1.893 → 1.499 → 1.237 → this). Threeindependent changes plus one refutation that closes the lever the previous round's
handoff was built around.
A — the strict class-field SET arm skipped the inline precheck for BOXED fields
expr/property_set.rsgatedemit_class_field_inline_precheckonrequires_raw_f64. Every store into a declared field that is not anumber— astring, a class type, a union: most fields of most objects — therefore paid anunconditional cross-crate
js_typed_feedback_class_field_set_guardcall. Thatincludes the synthesized
__AnonShape_*_constructorthat every closed-shape objectliteral runs, which is why
js_typed_feedback_class_field_set_guard(2.9%) andtyped_feedback::guards::class_field_fast_contract(2.1%) sat near the top ofinterp's profile:{ kind: "bin", op, left, right }is four stores, three boxed.The stated reason for the gate ("its setter-in-chain handling and write barrier
aren't reproduced inline") was already answered by
try_lower_sloppy_class_field_boxed_store, which has taken the boxed inlineprecheck since #7288 — the write barrier, layout note and string demote come from
emit_jsvalue_slot_store_pointer_tested, which the shared fast block calls with theidentical value-side predicates; a setter anywhere in the chain is refused upstream
by
class_field_global_index'saccessor_in_chain. The precheck proves a strictsubset of the runtime's
class_field_fast_contract, so on a hit the guard wouldhave answered "fast" too. Every miss still lands on the guardcall block and the
unchanged strict fallback.
Verified live rather than assumed:
interp.tsgoes from 3 to 44 emittedPERRY_CLASS_FIELD_INLINE_GUARD_DISABLEDgate loads (41 new prechecks) with thesame 43 guard-call sites, now on the miss arm.
B —
is_arguments_objecthad no emptiness latchIt is a probe, called from the by-name property-get tail,
Array.prototype.push,the array and
Symbol.iteratoriterator entries,Array.from/concatand classconstruction. In a program that never writes
argumentsit was still 2.8% ofinterp: a thread-local resolution (Darwin has no local-exec TLS, so that is a real_tlv_get_addr— 7.1% of the same profile), aRefCellborrow and a pointer hash,per call, to prove the absence of a feature the source does not contain.
ARGUMENTS_OBJECTS_EVER_USEDis a process-globalAtomicBoollatched by the oneand only registry insert and checked before the thread-local — the
EXTERNAL_BUFFERS_NONEMPTY/SET_REGISTRY_EVER_USEDidiom verbatim (#7474,#7469).
object/arguments_latch_tests.rsasserts the SUBJECT, not the answer:latch_off_is_what_makes_the_probe_cheapregisters a real arguments object, forcesthe latch back off, and requires the probe to answer
false— deliberately thewrong answer, and the only way to show the short-circuit is the arm being taken
rather than dead code in front of a registry that would have answered anyway. Delete
the early-out and it goes red.
C — a property read into an unannotated local threw its type away
const names = e.namesleftnamesatAny, sonames[i]lowered to ajs_dyn_index_getcall (4.3% ofinterp) whose own miss path callsjs_array_length(2.9%), andnames[i] === namelowered to the fully dynamicjs_eq.refine_type_from_init'sPropertyGetarm resolves the receiver withreceiver_class_name— which answersNonefor a reassigned local and for a union— and then looks only in
ctx.classes. A chain-walking cursor over atype Env = { … }alias therefore failed three separate ways.declared_property_type_from_annotationresolves through the same class / interface/ object-alias tables
static_type_ofalready consults, after strippingnull/undefinedfrom the receiver's union (a read that returns at all had anon-nullish receiver — reading through
nullthrows).★ It is a claim, not a proof — and one consumer could not take one
Element reads and stores tolerate a violated claim: both re-check
GC_TYPE_ARRAYand fall back.
.lengthdoes not. Its inline arm is guarded, but its fallbackjs_value_length_f64answers 0 for every value that carries no length, where JSanswers
undefined(and where a nullish receiver must throw). That degradation ispre-existing and documented in place in the runtime — and it is already reachable on
mainthrough a hand-written annotation:So
.lengthmust not be handed a fresh claim.refined_array_type_is_declared_onlyrecords these ids in
FnCtx::declared_only_array_locals— the mechanism #7773introduced for the numeric half — and the
.lengtharm refuses them, leaving themon exactly the generic path the unrefined
Anylocal takes today.test-files/test_gap_declared_field_type_refine_guarded.tsis the sabotage test:one
items: string[]declaration handed arrays, strings, plain objects apingarrays, numbers,
nullandundefined, through an alias, an interface and a class,via a nullable reassigned cursor, a nested read chain, an element store and an
element-typed
===/+. Every row must match node byte for byte. It was writtenbefore the guard and it failed — four rows read
len=0— which is how the hazardabove was found rather than shipped.
Measured — quiet M1 mini, best-of-5, exit-checked
origin/main@0321c6554(perry 0.5.1467) vs this branch. Every outputbyte-compared against
node --experimental-strip-typesbefore timing; exit coderecorded per cell. Load 1.48 before / 2.11 after, zero foreign processes at both ends.
Zero regressions. The fourteen programs sitting inside +/-0.5% are the run's own
noise floor (
medwithin 1 ms ofbestin every cell), which is what makes the threemovers credible. All protected ceilings met.
Correctness canaries:
interp.tsprints1708840;iso_miss.tsprintschecksum 437840 misses 0— gated on the miss counter, and re-verified underPERRY_GC_PROTECT_FROMSPACE=1 PERRY_GC_PROTECT_FROMSPACE_DEPTH=800 PERRY_GC_VERIFY_EVACUATION=1(both exit 0, output unchanged).cmpdoes not apply here: this touchesperry-runtime, so the two arms linkdifferent archives and all 19 binaries differ by construction.
Tests
cargo test --release -p perry-runtime arguments— 4 passed, including the two newlatch tests and the two existing
ARGUMENTS_OBJECTSGC-side-table tests.cargo test --release -p perry-codegen— all unit + integration suites pass exceptlarge_object_barriers::large_local_array_push_inbounds_store_emits_precise_slot_barrier,which is already red on
main@0321c6554and unrelated: perf(codegen): put the numeric array push's GC bookkeeping behind one live test (push_num 0.149 -> 0.069) #7839(
a64c5a9eb, the commit immediately before this branch point) movedjs_gc_note_slot_layout/js_write_barrier_slotout ofapush.inboundsinto anew
apush.gc_bookkeepingblock emitted afterapush.realloc, outside the slicethe old assertion searches. Verified empirically against a reference build of
0321c6554, not inferred: neither call appears betweenapush.inboundsandapush.realloc, andapush.gc_bookkeepingis present. perf(codegen): put the numeric array push's GC bookkeeping behind one live test (push_num 0.149 -> 0.069) #7839 wrote its ownreplacement gate and left this one stale; it landed green because
crates/*/tests/*.rsdo not run per-PR.cargo fmt --all -- --check,scripts/check_file_size.shclean.PERRY_SKIP_BUILD=1, node 26.5.1 per.node-version) — see the comment below for the outcome.Follow-up filed
#7853 —
.lengthon a receiver whose declared type is an array/Namedbut whoseruntime value carries no length returns
0instead ofundefined, and returns0where JS throws. Pre-existing on
main, reachable from a hand-written annotation, andthe reason change C has to carve
.lengthout. Fixing it removes the carve-out andrecovers the remaining ~3%.
Refuted: shape narrowing after a discriminant test
PROFILE-interp-round3.mdproposed narrowingnto its matching union memberinside
if (n.kind === "bin")soevalNode's surviving diamonds (27.2% of theprogram) become class-keyed slot loads. The ceiling was measured before building
it: ~5%, not the projected ~20%.
Three source-level arms of
interp.ts, built from one compiler and identical inevery other respect — object literals (the original); each union member as a real
class(isolating allocation from typing); that program plus hand-written narrowingcasts in every
evalNodearm. The narrowed arm converts 19 of 31 genericproperty diamonds into guarded class-field inline reads and is 1.226 → 1.162 s
against its own control, 5.2%. The reason is structural: a class-field guarded read
is only about a third cheaper than the polymorphic-IC read, because the cost is the
guard, not the lookup — narrowing swaps one guarded diamond for another. A large
win needs the check hoisted out of the branch (one shape test, N unguarded slot
loads), i.e. loop-versioning applied to a discriminant arm.